Skip to content

MQE-1918 MFTF AWS Secrets Manager - Local Use #543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

MQE-1918 MFTF AWS Secrets Manager - Local Use #543

wants to merge 6 commits into from

Conversation

jilu1
Copy link
Contributor

@jilu1 jilu1 commented Jan 10, 2020

Description

Fixed Issues (if relevant)

  1. magento/magento2-functional-testing-framework#<issue_number>: Issue title
  2. ...

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/verification tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
  • Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

@coveralls
Copy link

coveralls commented Jan 10, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.8%) to 52.726% when pulling 12c3bdf on MQE-1918 into 2e57c94 on develop.

@soumyau soumyau self-requested a review January 14, 2020 14:49
soumyau
soumyau suggested changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@soumyau soumyau left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if the credentials to the secret manager are not set correctly, the tests end up throwing exception like this -
MFTF uses Credential Storage in the following precedence: .credentials file, HashiCorp Vault and AWS Secret Manager. You need to configure at least one to use _CREDS in tests. And make sure key/value exists.
Not sure why it doesn't throw this error instead - Unable to create AWS Secret Manager client

eg: CREDENTIAL_AWS_SECRET_MANAGER_PROFILE=asdf (not existant)
CREDENTIAL_AWS_SECRET_MANAGER_REGION=us-east-2 (non existant)

soumyau
soumyau reviewed Jan 14, 2020
View reviewed changes
@jilu1
Copy link
Contributor Author

jilu1 commented Jan 14, 2020

One more thing, There is no error if you use non-existing profile when create AWS client. There will be error when access the secret. Thus the generic error message.

@soumyau
Copy link
Contributor

soumyau commented Jan 14, 2020 •
edited
Loading

One more thing, There is no error if you use non-existing profile when create AWS client. There will be error when access the secret. Thus the generic error message.

Since the CI/CD story may end up changing the order will it be better to modify the exception message to - You need to configure atleast one of these options: .credentials file, HashiCorp Vault or AWS Secrets Manager correctly and ensure key, value exists to use _CREDS in tests.

soumyau
soumyau reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks good. Needs branch update + a build trigger just to triple check

@jilu1 jilu1 force-pushed the MQE-1918 branch 2 times, most recently from aa287d2 to fd9e721 Compare January 17, 2020 22:07
@jilu1
Copy link
Contributor Author

jilu1 commented Jan 21, 2020

This is incorporated in PR # 554

@jilu1 jilu1 closed this Jan 21, 2020
@jilu1 jilu1 deleted the MQE-1918 branch February 6, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soumyau soumyau soumyau requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jilu1 @coveralls @soumyau